INTPYTHON-751 Make query generation omit $expr unless required #396

WaVEV · 2025-09-09T17:15:54Z

In this PR a unified approach for generating MQL from Django expressions was implemented. The core idea is to centralize the control flow in a base_expression method, which decides whether the expression can be translated into a direct field: value match (index-friendly) or must fall back to $expr. This keeps the logic for wrapping and dispatching in one place, while each lookup/function only defines its own expression-building logic.

This approach also allows mixing direct field: value matches with $expr clauses within the same $match. As a result, multiple $expr entries may coexist alongside index-optimized conditions, depending on the shape of the query.

Most lookups now follow this pattern by simply implementing as_mql_expr (and optionally as_mql_path when a match-based translation is possible). Only a few special cases like Col, Func operators (except the KeyTransform) , and many more, override the base behavior directly. This structure also leaves room for future optimizations (e.g. constant folding) without having to change the overall flow.

Additionally, since MongoDB 6 does not allow nesting $expr inside another $expr, the flow in base_expression ensures that such cases are flattened. In practice, expressions are generated without redundant wrapping, so the final MQL never contains $expr within $expr.

NOTE: Some polish will be made, but the main idea and the majority of the code is already rendered.

WaVEV · 2025-09-26T03:40:00Z

django_mongodb_backend/functions.py

-    Substr.as_mql = substr
-    Trim.as_mql = trim("trim")
-    TruncBase.as_mql = trunc
+    Cast.as_mql_expr = cast


All the function does not support as_mql_path. It could be added latter if we try to simplify constants expressions

WaVEV · 2025-09-26T04:33:56Z

django_mongodb_backend/expressions/builtins.py

    return value


+def base_expression(self, compiler, connection, as_path=False, **extra):


This is the common handler for all expressions. It defines if an expr is needed or not.

timgraham · 2025-09-26T23:34:24Z

django_mongodb_backend/fields/json.py

+    KeyTransformExact.as_mql_expr = key_transform_exact_expr
+    KeyTransformExact.as_mql_path = key_transform_exact_path


Please check alphabetization of the classes and functions (not only in this file).

timgraham · 2025-09-26T23:47:43Z

django_mongodb_backend/base.py

+    }
+
+    def range_match(a, b):
+        ## TODO: MAKE A TEST TO TEST WHEN BOTH ENDS ARE NONE. WHAT SHALL I RETURN?


AI says, "If either the start or end value provided to the BETWEEN operator is NULL, the entire BETWEEN condition will typically evaluate to UNKNOWN (and thus FALSE in a WHERE clause), unless explicitly handled." (I confirmed this for SQLite and PostgreSQL)

However, to match the semantics implemented here where None is treated as min/max date, I would expect __range=[None, None] not to filter any values.

Raise FullResultSet is the way to go, and EmptyResultSet if the low is bigger than high.

django_mongodb_backend/aggregates.py

timgraham · 2025-09-27T00:03:27Z

django_mongodb_backend/aggregates.py

+    Aggregate.as_mql_expr = aggregate
+    Count.as_mql_expr = count
+    StdDev.as_mql_expr = stddev_variance
+    Variance.as_mql_expr = stddev_variance


I see we have as_mql_expr(), as_mql_path(), and as_mql(..., as_path=...). If this is the way we keep it, it would be good to explain in the design document which objects (aggregate, func, expression, etc.) get which.

I wonder about renaming as_mql_expr() or as_mql_path() to as_mql() (i.e. treating one of paths as the default). Do you think it would be more or less confusing?

Yes, that was the idea. I’ll explain it in the docs, and we might also consider renaming some methods. The core concept is:

Every expression has an as_mql method.

In some cases, it’s simpler to implement as_mql directly, so those methods don’t follow the common expression flow.

For other expressions, as_mql is a composite function that delegates to as_path or as_expr when applied.

The base_expression.as_mql method controls when these are called and performs boilerplate checks to prevent nesting an expr inside another expr (a MongoDB 6 restriction).

In short: every object has as_mql. Some also define as_path and as_expr. The base_expression coordinates how these methods are used, except for cases where as_mql is defined directly.

Doc here: link

@timgraham I actually like the decoupling of as_mql and as_mql_(path | expr). I view it as, you need to define at least two:
as_mql and as_mql_expr.

Then if you want to have the more optimized function you define as_mql_path. It feels less confusing to me that way.

🤔 An expression needs at least one of the methods, as_mql_expr is enough (not optimal but functional). The aggregation module is a good example. Because the as_mql is defined in baseExpression class. In this approach most of the expressions don't need as_mql (if as_mql is defined, expr or path aren't needed).
EDIT: Sorry, I rushed the answer before read all the text, you got the point well. 😬

django_mongodb_backend/base.py

django_mongodb_backend/functions.py

django_mongodb_backend/query.py

django_mongodb_backend/query_utils.py

tests/model_fields_/test_embedded_model.py

tests/model_fields_/test_embedded_model_array.py

django_mongodb_backend/base.py

WaVEV · 2025-10-04T16:22:08Z

django_mongodb_backend/base.py

+            conditions.append({a: {"$lte": b[1]}})
+        if start is not None and end is not None:
+            if isinstance(start, Decimal128):
+                start = start.to_decimal()


potencial overflow risk here?

Is there a failing test without the to_decimal() calls? I'm not sure why there would be a mix of Decimal/Decimal128 here.

This is a BSON type held at the database and returned locally. I think it should be good.

yes, there is: model_fields_.test_polymorphic_embedded_model.QueryingTests.test_range. Maybe this case shouldn't be handled here, and let the query optimizer to remove those False cases.

Jibola

Left my first round of comments. I'm going to review the tests as a second phase of the PR.

Jibola · 2025-10-06T13:34:11Z

django_mongodb_backend/base.py

+        if b:
+            return {"$or": [{a: {"$exists": False}}, {a: None}]}


What does b represent in this case? Not negating?

🤔 well, it is a bit confusing here. but this function checks for nullability. So, if b is True then we check null. maybe the parameter should be: field, null.

field and is_null are good clarifiers

django_mongodb_backend/base.py

Jibola · 2025-10-06T13:52:46Z

django_mongodb_backend/aggregates.py

+    Aggregate.as_mql_expr = aggregate
+    Count.as_mql_expr = count
+    StdDev.as_mql_expr = stddev_variance
+    Variance.as_mql_expr = stddev_variance


@timgraham I actually like the decoupling of as_mql and as_mql_(path | expr). I view it as, you need to define at least two:
as_mql and as_mql_expr.

Then if you want to have the more optimized function you define as_mql_path. It feels less confusing to me that way.

Jibola · 2025-10-06T14:49:06Z

django_mongodb_backend/base.py

+            conditions.append({a: {"$lte": b[1]}})
+        if start is not None and end is not None:
+            if isinstance(start, Decimal128):
+                start = start.to_decimal()


This is a BSON type held at the database and returned locally. I think it should be good.

Jibola · 2025-10-06T14:55:39Z

django_mongodb_backend/expressions/search.py

+    def as_mql_expr(self, compiler, connection):
+        lhs_mql = process_lhs(self, compiler, connection, as_path=False)
+        value = process_rhs(self, compiler, connection, as_path=False)
        return {"$gte": [lhs_mql, value]}

+    def as_mql_path(self, compiler, connection):
+        lhs_mql = process_lhs(self, compiler, connection, as_path=True)
+        value = process_rhs(self, compiler, connection, as_path=True)
+        return {lhs_mql: {"$gte": value}}
+


Why is there a $gte query in a search.text lookup?

To convert a score function into a filter I decided to express the following proposition: score_func(...) > 0.

Jibola · 2025-10-06T19:36:33Z

django_mongodb_backend/fields/embedded_model_array.py

+        return self.is_simple_column
+
+    @cached_property
+    def is_simple_column(self):


I see this validation for embedded models used in multiple places. Can you consolidate this to the query_utils file?

🤔 will try. The overall structure of the function is similar, but the type varies. I could create a meta-function that generates the appropriate function for a given type.

I didn't get better. 😬 . I will let as it is. there is some details like:
previous._field.column, previous.key_name when extract the field_name or the path. Then are different from EMFA and EMF the return, one has to validate the inner transform while the other doesn't

Jibola · 2025-10-06T20:53:47Z

django_mongodb_backend/compiler.py

            where = self.get_where()
            try:
-                expr = where.as_mql(self, self.connection) if where else {}
+                match = where.as_mql(self, self.connection, as_path=True) if where else {}


Since we enforce as_path=True here, can we set that as the default on WhereNode.as_mql(..., as_path=True)?

Yes, will refactor. Also it may change the name, as_expr=False. but it is scenically the same

Jibola · 2025-10-06T21:13:59Z

django_mongodb_backend/query_utils.py



-def process_lhs(node, compiler, connection):
+def process_lhs(node, compiler, connection, as_path=False):


Based on the delegation logic, should as_path always default to True?

If the caller requests a path filter (as_path=True) and the expression supports it, as_mql_path is executed.
If the expression cannot be represented as a path filter but as_path=True, it is automatically encapsulated in $expr.
If the caller requests an expression (as_path=False), as_mql_expr is executed directly. No additional encapsulation is needed, since the query generation ensures proper nesting of $expr.

Jibola · 2025-10-06T21:16:08Z

django_mongodb_backend/query_utils.py

+    if isinstance(value, CombinedExpression):
+        # Temporary: treat all CombinedExpressions as non-constant until
+        # constant cases are handled


Can you make a follow-up ticket for this? If it already exists, could you link it in the comment?

Yes, I will.

https://jira.mongodb.org/browse/INTPYTHON-783

Jibola · 2025-10-06T21:47:23Z

django_mongodb_backend/query_utils.py

+
+
+def valid_path_key_name(key_name):
+    return bool(re.fullmatch(r"[A-Za-z0-9_]+", key_name))


https://www.mongodb.com/docs/manual/core/dot-dollar-considerations/

Values like hashtags are also valid for path names and don't require $expr. To my knowledge so long as it's not (.) or ($) it's good.

Thanks, will adjust the expression.

what if there is some emoji or some non ascii character? 🤔

Jibola · 2025-10-06T22:28:04Z

django_mongodb_backend/fields/embedded_model.py

+    def as_mql_expr(self, compiler, connection):
+        columns, parent_field = self._get_target_path()
+        mql = parent_field.as_mql(compiler, connection)
+        for key in columns:
+            mql = {"$getField": {"input": mql, "field": key}}


potentially out of scope:
https://github.com/mongodb/django-mongodb-backend/pull/392/files#diff-0a6ce30a131a00fa88086c4c4d0d6e6232845fd11ef2bc67891fdf92e10c3743R18-R45

Is it possible to still remove $getField in as_mql_expr or is it expected that routing to as_mql_expr for embedded model queries is because of needing a getField call?

Jibola · 2025-10-06T22:31:21Z

django_mongodb_backend/lookups.py

+@property
+def can_use_path(self):
+    simple_column = getattr(self.lhs, "is_simple_column", False)
+    constant_value = is_constant_value(self.rhs)
+    return simple_column and constant_value


Jibola · 2025-10-06T23:31:24Z

django_mongodb_backend/base.py

+        if b:
+            return {"$or": [{a: {"$exists": False}}, {a: None}]}


field and is_null are good clarifiers

Jibola · 2025-10-06T23:41:13Z

tests/model_fields_/test_arrayfield.py

+        self.assertAggregateQuery(
+            query,
+            "model_fields__nullableintegerarraymodel",
+            [{"$match": {"field": {"$in": ([1], [2])}}}],


Does $in now expect a tuple?

Yes, but it wasn’t a new convention or expectation. There was already a test that checks the RHS $in as a tuple, so I just followed that convention. It doesn’t affect the query behavior.

Jibola · 2025-10-06T23:44:30Z

tests/model_fields_/test_embedded_model.py

+                    "$match": {
+                        "$expr": {
+                            "$eq": [
+                                {"$getField": {"input": "$data", "field": "integer_"}},


This is an example of a value that could get rid of the getField.

Yes, we could get rid of those getField if we use $data.integer_ but I thought it was out of scope for this refactor. This behavior is the current behavior.

Yes, it's out of scope!

Jibola · 2025-10-06T23:47:21Z

tests/model_fields_/test_embedded_model.py

+            [
+                {
+                    "$match": {
+                        "$expr": {


Curious to the callback chain on this one since the null-check could actually be converted.

May best be an improvement added later

…set in range queries.

WaVEV force-pushed the lookup-refactor branch 3 times, most recently from 529e0ff to a78f26b Compare September 15, 2025 21:21

timgraham changed the title ~~WIP lookup refactor~~ INTPYTHON-751 Make query generation omit $expr unless required Sep 20, 2025

WaVEV force-pushed the lookup-refactor branch from 8b0c247 to 141f1cf Compare September 23, 2025 03:52

Jibola mentioned this pull request Sep 23, 2025

INTPYTHON-750: Support converting $expr with $getField paths #392

Closed

WaVEV force-pushed the lookup-refactor branch from d11378a to 2c48d11 Compare September 26, 2025 02:26

WaVEV commented Sep 26, 2025

View reviewed changes

WaVEV marked this pull request as ready for review September 26, 2025 04:52

WaVEV requested review from Jibola and timgraham and removed request for Jibola September 26, 2025 04:52

timgraham reviewed Sep 27, 2025

View reviewed changes

WaVEV commented Sep 27, 2025

View reviewed changes

tests/model_fields_/test_embedded_model_array.py Show resolved Hide resolved

WaVEV force-pushed the lookup-refactor branch 4 times, most recently from 5827580 to 5b9fa93 Compare October 4, 2025 02:50

WaVEV commented Oct 4, 2025

View reviewed changes

django_mongodb_backend/base.py Outdated Show resolved Hide resolved

WaVEV force-pushed the lookup-refactor branch from 5b9fa93 to b0068d4 Compare October 4, 2025 03:07

WaVEV commented Oct 4, 2025

View reviewed changes

WaVEV force-pushed the lookup-refactor branch from 7469207 to 9be75aa Compare October 5, 2025 18:43

Jibola requested changes Oct 6, 2025

View reviewed changes

WaVEV force-pushed the lookup-refactor branch from 9be75aa to cd47412 Compare October 6, 2025 21:49

Jibola reviewed Oct 6, 2025

View reviewed changes

WaVEV added 3 commits October 7, 2025 00:35

Functional approach solution

4e4ed59

Object-oriented approach solution

f87fac0

Clean ups.

a035274

WaVEV added 4 commits October 7, 2025 00:35

Add mql check in EMF and EMFA unit test and handle empty set or full …

bc7e8b6

…set in range queries.

Rename operators mapping.

78cde50

Refactor: replace es_path for as_expr.

8572489

rename parameter.

8a8b441

WaVEV force-pushed the lookup-refactor branch from 55eae28 to 8a8b441 Compare October 7, 2025 03:48

timgraham mentioned this pull request Oct 9, 2025

Rename DatabaseWrapper operator helpers #417

Open

		return value


		def base_expression(self, compiler, connection, as_path=False, **extra):

		KeyTransformExact.as_mql_expr = key_transform_exact_expr
		KeyTransformExact.as_mql_path = key_transform_exact_path



		def process_lhs(node, compiler, connection):
		def process_lhs(node, compiler, connection, as_path=False):



		def valid_path_key_name(key_name):
		return bool(re.fullmatch(r"[A-Za-z0-9_]+", key_name))

INTPYTHON-751 Make query generation omit $expr unless required #396

Are you sure you want to change the base?

INTPYTHON-751 Make query generation omit $expr unless required #396

Uh oh!

Conversation

WaVEV commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WaVEV Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WaVEV Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jibola left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

WaVEV commented Sep 9, 2025 •

edited

Loading

WaVEV Sep 27, 2025 •

edited

Loading

WaVEV Oct 6, 2025 •

edited

Loading